Conversation
priettt
force-pushed
the
priettt/injectBundleIdWithASM
branch
2 times, most recently
from
bccae37 to
2423152
Compare
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
priettt
force-pushed
the
priettt/injectBundleIdWithASM
branch
2 times, most recently
from
cb78816 to
06fa5d9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2199 +/- ##
==========================================
- Coverage 85.63% 85.63% -0.01%
==========================================
Files 465 465
Lines 10389 10381 -8
Branches 1522 1522
==========================================
- Hits 8897 8890 -7
- Misses 835 836 +1
+ Partials 657 655 -2
🚀 New features to boost your workflow:
|
priettt
force-pushed
the
priettt/injectBundleIdWithASM
branch
2 times, most recently
from
e8397c2 to
4b01062
Compare
bidetofevil
reviewed
May 9, 2025
| import io.embrace.android.gradle.plugin.instrumentation.config.model.VariantConfig | ||
|
|
||
| fun createProjectConfigInstrumentation(cfg: VariantConfig) = modelSdkConfigClass { | ||
| fun createProjectConfigInstrumentation(cfg: VariantConfig, reactNativeBundleId: String?) = modelSdkConfigClass { |
Contributor
There was a problem hiding this comment.
Any reason why this is parallel to the config object rather than part of it?
It's not part of the Embrace config that we derive from the config file, but it certain seems parallel to things like buildId
Contributor
Author
There was a problem hiding this comment.
it's mostly because we can't access it at the same time, but it'd be nice to aggregate the parameters in a class in the future, especially in createClassVisitor.
bidetofevil
approved these changes
May 9, 2025
Contributor
bidetofevil
left a comment
bidetofevil
left a comment
There was a problem hiding this comment.
LGTM. One question about why the bundleId doesn't live in the same object as the buildId
priettt
force-pushed
the
priettt/injectBundleIdWithASM
branch
from
4b01062 to
e01ee51
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Similar to what we did with NDK symbols, inject the React BundleID with ASM. This allows us to remove resource injection altogether and access the bundle ID through the project instrumented config.
Testing
Modified integration test to verify injection